Skip to content

[feat/refactor] AllColumns and AllTableColumns-Support for JSON_OBJECT #2323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Oct 12, 2025
Merged

[feat/refactor] AllColumns and AllTableColumns-Support for JSON_OBJECT #2323

merged 7 commits into from
Oct 12, 2025

Conversation

ANeumann82
Copy link
Contributor

This adds support for the JSON_OBJECT(*) and JSON_OBJECT(t1.*, t2.*) syntax, reference is here: https://docs.oracle.com/en/database/oracle/oracle-database/23/sqlrf/JSON_OBJECT.html

While working on this, I noticed that the current handling is a bit off:
In OBJECT_POSTGRES mode, the separate entries are stored in key and value of a single keyValue, and the AllColumns and AllTableColumns-Objects would end up there as well.

As it is possible to mix actual key value pairs and AllTableColumns ( JSON_OBJECT(t1.*, 'name' : t2.name, t3.*) ) we need to make sure that each "entry" gets it's own KeyValuePair, with just the key (or value) set.

I'm not sure how breaking changes are handled here. The PR so far is binary compatible, i.e. the API is backwards compatible, but the behaviour changed slightly.

OBJECT_MYSQL and OBJECT_POSTGRES are not used anymore. Instead the JsonKeyValuePair now has a "separator"-Field that describes how the key and the value are rendered. There is one entry NOT_USED in case the KeyValuePair has only one value.

Open questions:

  • Should we store single entry keyValuePairs in the key or the value field. Value seems a bit more sane, but key is easier.
  • Is it ok to deprecate the old syntax ( JsonKeyValuePair.get/set/withValueKeyword, JsonFunctionType.OBJECT_MYSQL, etc.)? I assume they shouldn't simply be deleted.

I plan to work on this a bit more, also the JsonAggregateFunction should probably be adapted as well, but I'd like to get your opinion on the whole thing first.

@ANeumann82 ANeumann82 marked this pull request as ready for review October 11, 2025 12:27
@ANeumann82
Copy link
Contributor Author

Did a bit more work here, and I think it's ok for now. Some thoughts:

  • MySQLs syntax that allows separation of key and value with a ',' is stupid
  • Informix and Snowflake syntax of using ':' to extract values from JSON clashes with the key value separation in all other syntaxes. I would prefer to have the Informix and Snowflake syntax behind a feature flag, but it's supported at the moment and should probably stay that way for now.

I see a lot of potential issues with incompatible syntaxes in the future, we should probably think about how to solve them. I know that the library is designed as a general, language agnostic parser, but at some point it's simply not possible to support everything without specifying the expected syntax...

@ANeumann82
Copy link
Contributor Author

JMH on master (without this PR):

Benchmark                               (version)  Mode  Cnt    Score    Error  Units
JSQLParserBenchmark.parseSQLStatements     latest  avgt   15  147.645 ▒ 18.556  ms/op
JSQLParserBenchmark.parseSQLStatements        5.3  avgt   15  130.678 ▒ 11.676  ms/op
JSQLParserBenchmark.parseSQLStatements        5.1  avgt   15  106.721 ▒ 11.682  ms/op

JMH on this PR:

Benchmark                               (version)  Mode  Cnt    Score    Error  Units
JSQLParserBenchmark.parseSQLStatements     latest  avgt   15  147.743 ▒ 16.034  ms/op
JSQLParserBenchmark.parseSQLStatements        5.3  avgt   15  133.559 ▒ 10.180  ms/op
JSQLParserBenchmark.parseSQLStatements        5.1  avgt   15  113.411 ▒ 12.000  ms/op

@manticore-projects
Copy link
Contributor

Thank you for your work and effort. No objections from my side and I will merge when you are ready.

@ANeumann82
Copy link
Contributor Author

Thank you, please go ahead and merge, this PR is good from my side. I'll probably have a look at JSON_TABLE next

@manticore-projects manticore-projects merged commit e8d6058 into JSQLParser:master Oct 12, 2025
2 of 3 checks passed
@manticore-projects
Copy link
Contributor

Next time please check for those warnings too:

Warning: Choice conflict involving two expansions at
         line 7016, column 14 and line 7020, column 14 respectively.
         A common prefix is: "*"
         Consider using a lookahead of 2 for earlier expansion.
Warning: Choice conflict involving two expansions at
         line 7018, column 14 and line 7020, column 14 respectively.
         A common prefix is: <DATA_TYPE> "..."
         Consider using a lookahead of 3 or more for earlier expansion.

@ANeumann82
Copy link
Contributor Author

Next time please check for those warnings too:

Warning: Choice conflict involving two expansions at
         line 7016, column 14 and line 7020, column 14 respectively.
         A common prefix is: "*"
         Consider using a lookahead of 2 for earlier expansion.
Warning: Choice conflict involving two expansions at
         line 7018, column 14 and line 7020, column 14 respectively.
         A common prefix is: <DATA_TYPE> "..."
         Consider using a lookahead of 3 or more for earlier expansion.

Yeah, didn't look for the JavaCC-Warnings. I'll check those and make another PR to fix them

@manticore-projects
Copy link
Contributor

No worries, I did that already. Good team effort: you do the heavy lifting and leave the easy work to me. Cheers and best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ANeumann82 @manticore-projects